Extend semantic conventions for RPC and allow non-gRPC calls #604
Conversation
arminru
requested review from
bogdandrutu,
c24t,
carlosalberto,
iredelmeier,
jmacd,
reyang,
SergeyKanzhelev,
tedsuo,
tigrannajaryan and
yurishkuro
as code owners
|
@open-telemetry/specs-approvers Anyone willing to have a look? 🙂 |
| * [Status](#status) | ||
| * [Events](#events) | ||
|
|
||
| <!-- tocstop --> | ||
|
|
||
| ## gRPC | ||
| ## Common remote procedure call conventions |
There was a problem hiding this comment.
Do you suggest to shorten the heading or are you questioning the wording?
| | `rpc.service` | The full name of the service being called, including its package name, if applicable. | Yes | | ||
| | `rpc.method` | The name of the method being called, must be equal to the $method part in the span name. | Yes | |
There was a problem hiding this comment.
Do we really need these conventions? Looks like duplicate information with the name. I would definitely not make them required.
There was a problem hiding this comment.
rpc.system is not part of the span name
rpc.service was already there, I think it was taken over from OpenTracing where it was called peer.service
rpc.method is new
While the latter two are part of the suggested span name, I'd rather have them available without having to parse the name and decompose the components.
On all other semantic conventions the most valuable data, which is used to detect and distinguish services, is provided using attributes and not from parsing the span name, even though this results in some duplication.
I deem the span name less reliable since it's intended as a grouping key and/or to provide a human-readable friendly name rather than a definite, structured data format. I'd expect deviations from the span name guidelines defined in the spec more frequently than not adhering to the attributes defined therein. For example, one could add a "RPC call to xy" prefix to the span name above to "make it look nicer in the UI" even though this would not be allowed as per the spec.
There was a problem hiding this comment.
Not sure I can buy the "send duplicate data" to avoid "parsing". How do we know which one is more expensive? I would simply not make them required.
There was a problem hiding this comment.
Avoiding parsing is one of three reasons I stated in my comment above.
The main reason is that it's in my opinion the least reliable data point we have on a span. All other semantic conventions say something along "you should use this span name or alternatively that but you can also use something else if it suits better or is of lower cardinality", so I personally doubt that users/instrumenters will adhere 100% to the guidelines laid out regarding span names. As long as it still makes a good, low-cardinality grouping key, this would be mostly fine, however.
For detecting services from span, I certainly prefer well-defined unambiguous attributes over something that is the main identifier showing up in the UI. The human-readable span name in the UI might, IMHO, be more likely to be populated with something else, because it's more favorable, self-explaining, human-readable or otherwise preferred by users looking at the UI (even if the spec would be pretty strict about it in this particular semantic convention).
There was a problem hiding this comment.
My fight is still that this should not be required. I am fine and would approve this change if that is removed.
There was a problem hiding this comment.
I feel that required means will not be able to consume data without this field, which is not the case for this.
There was a problem hiding this comment.
Alright, I'll make it recommended instead of required and we can revisit it if #653 brings any applicable changes to the definition of required.
There was a problem hiding this comment.
@bogdandrutu I made rpc.method not required. Should I also remove the requirement for rpc.service that is already in place in the current spec version or leave it as it is?
There was a problem hiding this comment.
Yes please rpc.service should be the same.
OffTopic: Probably we should have 3 options (we can discuss in an issue OPTIONAL, RECOMMENDED, REQUIRED) then we should have very few `REQUIRED.
There was a problem hiding this comment.
@bogdandrutu done.
Your proposal sounds reasonable, we should discuss this on #653.
|
Had one thought about rpc.service. I think in tracing systems, having a good service name is very important for features like dependency graphs, and this applies to all spans. Does it make sense to move The example I am thinking of is Redis. There is no descriptive name for a redis database, it's up to how the user code uses it, so a redis may be named |
|
@anuraaga: @yurishkuro noted in #604 (comment) that they used a |
…te from rpc.service
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
…lemetry#604) * Generalize RPC semantic conventions to allow non-gRPC spans * Add method name to attributes * Require specifying `net.transport` for non-IP connections * Do not require providing the port when it's not available * Add package name to rpc.service and make it optional when not applicable or unknown * Add note and example distinguishing the service.name resource attribute from rpc.service * Distinguish RPC spans from HTTP spans * Improve wording Co-authored-by: Christian Neumüller <christian+github@neumueller.me> * Improve wording * Improve wording even more Co-authored-by: Christian Neumüller <christian+github@neumueller.me> * Clarify span name format * Update changelog * rpc.method: required -> recommended * rpc.service: required -> recommended Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
I generalized the semantic conventions to allow RPC systems other than gRPC.
Therefore, I added an attribute
rpc.system(like we have for messaging systems asmessaging.systemhere) to identify the RPC system being used and added a section for gRPC, definingrpc.system="grpc"in this case.Once we use proper YAML definitions, we can add a list of well-known descriptors to be used in addition to the ones I mentioned as examples.
Additionally, I added the method name as an attribute so that it doesn't have to be parsed out of the span name. This also makes it available for sampling decisions since the span name is not reliable to use here (#468).
By requiring the
net.transportattribute to be set for non-IP connections we ensure that connections via named pipe bindings, for example, can be properly detected.I didn't touch the
rpc.serviceattribute already present but was wondering if we should change it to be the fully-qualified service name including $package, rather than just the unqualified name. What do you think?I'm also considering to add an attribute for the logical endpoint which might be different from the physical endpoint described using the
net.peer.*attributes. This would be particularly significant in scenarios where a service registry is used to resolve endpoints. We can either add it here or discuss it separately on a follow-up PR.Resolves #27.